Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

More create disk cleanup #760

Closed
wants to merge 4 commits into from

Conversation

cgwalters
Copy link
Member

No description provided.

As far as I can tell, we always passed this as a parameter but
as a fixed value.  Just hardcode it.  Prep for further cleanups.
We're always writing to that disk.
This doesn't change the use of `runvm` for the oscontainer command,
but in a metal/qemu build we can more conveniently have it look for
data in the temp build directory.
Now that `runvm` uses the current working directory, convert
the `image.yaml` to JSON so it can be conveniently and directly
parsed via `jq` in shell in `create_disk.sh`.

General cleanup in prep for adding more to `create_disk.sh`.
@ajeddeloh
Copy link
Contributor

When I wrote create_disk.sh I was trying to avoid implicitly depending on files already in places or other variables. I wanted it to be explicit about all of it's inputs and outputs. I agree we ought to clean up how they're handled, but I think I want to keep it explicit. Thoughts?

On that note, I'd like to also spend some time refactoring some of cosa to rely less on special locations and global variables, or at least make it very clear what they are (e.g. have them all defined in the same place with comments, rather than progressively throughout the codebase).

@cgwalters
Copy link
Member Author

I think of create_disk.sh as part of cosa - our inputs are pretty well defined in terms of the config git right?

@cgwalters
Copy link
Member Author

Would it address your concern if we moved the /usr/lib/coreos-assembler/grub.cfg back to the toplevel, something like

diff --git a/src/create_disk.sh b/src/create_disk.sh
index fa7ac23a..a55f039e 100755
--- a/src/create_disk.sh
+++ b/src/create_disk.sh
@@ -20,12 +20,14 @@ export PATH=$PATH:/sbin:/usr/sbin
 arch="$(uname -m)"
 
 disk=/dev/vda
+grub_cfg=/usr/lib/coreos-assembler/grub.cfg
 
 ostree="$1" && shift
 ref="$1" && shift
 os_name="$1" && shift
 extrakargs="$1" && shift
 
+# These are from the config image.yaml
 remote_name=$(jq -r '.["ostree-remote"]' < tmp/image.json)
 save_var_subdirs=$(jq -r '.["save-var-subdirs"]' < tmp/image.json)
 
@@ -125,7 +127,7 @@ normal
 EOF
 
 	# copy the grub config and any other files we might need
-	cp /usr/lib/coreos-assembler/grub.cfg rootfs/boot/grub2/grub.cfg
+	cp "${grub_cfg}" rootfs/boot/grub2/grub.cfg
 else
 	# current zipl expects 'title' to be first line, and no blank lines in BLS file
 	# see https://github.com/ibm-s390-tools/s390-tools/issues/64

?

@@ -402,6 +402,7 @@ runvm() {
set -xeuo pipefail
export PATH=/usr/sbin:$PATH
workdir=${workdir}
builddir=$(pwd)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's rename this e.g. host_pwd=$(pwd) ?

IOW, I don't think we should think of this as a builddir since we don't always have one. Whereas if we just reframe it as "make supermin command run in the same dir as where we were", that achieves the same goal and seems a less surprising behaviour than always cd'ing to the workdir.

Let's maybe also add a check that the host_pwd is under workdir and just error out otherwise? That way we ensure that we end up under the workdir mount.

@ajeddeloh
Copy link
Contributor

I more imagined create_disk being like a standalone script; it's honestly the direction I hoped cosa would have taken in general, especially with supermin. The codebase has grown rather organically and it's become really hard to keep track of all the globals and workdir/pwd/tmpdir/etc.

It's more of a preference thing, but I much prefer explicit passing of all the parameters than relying on things being at special locations. The caller should be responsible for knowing where things are, which block device to use, etc; The callee shouldn't need to know that. Plus if we need to change any of those things in the future, this is already parameterized. I think of it more as a tool with options; it's a bad analogy by fdisk would never assume a block device.

If we want to clean up argument passing things that are just copied from image.yaml how about converting it to json (as this does already) and passing that in but still having a parameter to say where that file is. e.g: create_dish.sh <other args> path/to/image.json?

@cgwalters
Copy link
Member Author

Well I was doing this as prep for #768
but trying to do more refactoring here just didn't end up any better. Ultimately I think this will only be cleaner with "not shell script" as a language.

@cgwalters cgwalters closed this Sep 20, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants